Skip to content

WPB-28422 reconcile stale local memberships for deleted remote conversations - #5504

Merged
battermann merged 6 commits into
developfrom
WPB-28422-backend-reconcile-stale-local-memberships-for-deleted-remote-conversations
Sep 10, 2026
Merged

battermann merged 6 commits into
developfrom
WPB-28422-backend-reconcile-stale-local-memberships-for-deleted-remote-conversations

Conversation

@battermann

@battermann battermann commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

https://wearezeta.atlassian.net/browse/WPB-28422

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@battermann
battermann marked this pull request as ready for review September 1, 2026 14:13
@battermann
battermann requested review from a team as code owners September 1, 2026 14:13
@battermann
battermann requested a lite review from Copilot September 1, 2026 14:14
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Sep 1, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces reconciliation logic to clean up stale local membership records when a remote (federated) conversation is definitively absent from the owning backend’s successful response, and adds integration coverage for this behavior.

Changes:

  • Reconcile (delete) stale local memberships when a remote “get conversations” call succeeds but omits locally-tracked remote conversation IDs.
  • Add integration tests that validate reconciliation behavior (including idempotency) and ensure memberships are preserved on federation failures.
  • Update an existing integration test to tolerate “no-conversation” results for a one-to-one conversation after remote user deletion.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
libs/wire-subsystems/src/Wire/ConversationSubsystem/Query.hs Adds reconciliation side-effect when remote conversations are missing from a successful federated response.
integration/test/Test/Conversation.hs Adds integration tests for stale membership reconciliation and adjusts expectations for one-to-one conversation retrieval after remote user deletion.
changelog.d/6-federation/WPB-28422 Documents the new stale-membership cleanup behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread libs/wire-subsystems/src/Wire/ConversationSubsystem/Query.hs Outdated
@battermann
battermann force-pushed the WPB-28422-backend-reconcile-stale-local-memberships-for-deleted-remote-conversations branch from 256eb56 to 0caac72 Compare September 1, 2026 14:31
@battermann
battermann requested a lite review from Copilot September 1, 2026 14:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

handleFailure (Right c) = pure . Right . traverse (.convs) $ c
handleFailure locallyFound (Right response) = do
let locallyFoundForDomain = filter ((== tDomain response) . tDomain) locallyFound
returnedIds = Set.fromList $ map (qualifyAs response . (.id)) (tUnqualified response).convs

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks too complicated to unqualified and then qualify, doesn't Remote a have a functor?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, the problem is, we need a Set (Remote ConvId) and not Remote (Set ConvId), and I think there is no built in generic operation that does this.

Comment thread libs/wire-subsystems/src/Wire/ConversationSubsystem/Query.hs Outdated
Comment on lines 544 to 547
unless (null remoteNotFoundRemoteIds) $
-- FUTUREWORK: This implies that the backends are out of sync. Maybe the
-- current user should be considered removed from this conversation at this
-- point.
P.warn $
Logger.msg ("Some locally found conversation ids were not returned by remotes" :: ByteString)
. Logger.field "convIds" (show remoteNotFoundRemoteIds)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need any of this again here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is calling the code above.

Comment thread libs/wire-subsystems/src/Wire/ConversationSubsystem/Query.hs Outdated
@fisx fisx added bug ready-for-review looking actively for reviewer and removed bug labels Sep 9, 2026

@blackheaven blackheaven left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need an extra non-regression tests:

-- | Fetching stale remote conversations from two different domains in one
-- request reconciles both independently and only from successful responses.
testReconcileStaleMembershipsMultipleDomains :: (HasCallStack) => App ()
testReconcileStaleMembershipsMultipleDomains = do
  resourcePool <- asks resourcePool
  runCodensity (acquireResources 1 resourcePool) $ \[remoteBackend] ->
    runCodensity (startDynamicBackend remoteBackend mempty) $ \_ -> do
      alice <- randomUser OwnDomain def
      ownerStatic <- randomUser OtherDomain def
      ownerDynamic <- randomUser remoteBackend.berDomain def
      connectTwoUsers ownerStatic alice
      connectTwoUsers ownerDynamic alice
      convStatic <- registerMissingRemoteConversation ownerStatic [alice]
      convDynamic <- registerMissingRemoteConversation ownerDynamic [alice]
      eventually $ do
        assertConversationMembership alice convStatic True
        assertConversationMembership alice convDynamic True

      bindResponse (listConversations alice [convStatic, convDynamic]) $ \resp -> do
        resp.status `shouldMatchInt` 200
        resp.json %. "found" `shouldMatch` ([] :: [Value])
        resp.json %. "failed" `shouldMatch` ([] :: [Value])
        notFound <- resp.json %. "not_found" & asList
        for_ [convStatic, convDynamic] $ \conv -> do
          (notFound :: [Value]) `shouldContain` [conv]

      assertConversationMembership alice convStatic False
      assertConversationMembership alice convDynamic False

-- | Conversations the remote still returns are preserved; only omitted ones
-- are reconciled within the same request.
testReconcileOnlyMissingConversations :: (HasCallStack) => App ()
testReconcileOnlyMissingConversations = do
  alice <- randomUser OwnDomain def
  owner <- randomUser OtherDomain def
  connectTwoUsers owner alice

  alive <-
    postConversation owner (defProteus {qualifiedUsers = [alice]})
      >>= getJSON 201
  aliveQid <- objQidObject alive
  stale <- registerMissingRemoteConversation owner [alice]

  eventually $ do
    assertConversationMembership alice aliveQid True
    assertConversationMembership alice stale True

  bindResponse (listConversations alice [aliveQid, stale]) $ \resp -> do
    resp.status `shouldMatchInt` 200
    resp.json %. "failed" `shouldMatch` ([] :: [Value])
    found <- resp.json %. "found" & asList
    length (found :: [Value]) `shouldMatchInt` 1
    notFound <- resp.json %. "not_found" & asList
    (notFound :: [Value]) `shouldContain` [stale]

  assertConversationMembership alice aliveQid True
  assertConversationMembership alice stale False

Comment thread libs/wire-subsystems/src/Wire/ConversationSubsystem/Query.hs Outdated
Comment thread libs/wire-subsystems/src/Wire/ConversationSubsystem/Query.hs Outdated
@fisx fisx removed the ready-for-review looking actively for reviewer label Sep 10, 2026
@fisx fisx added the ready-for-review looking actively for reviewer label Sep 10, 2026
@battermann
battermann merged commit 02557c5 into develop Sep 10, 2026
9 checks passed
@battermann
battermann deleted the WPB-28422-backend-reconcile-stale-local-memberships-for-deleted-remote-conversations branch September 10, 2026 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist ready-for-review looking actively for reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants